Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix storage layer limit #11156

Merged
merged 2 commits into from
Apr 4, 2022
Merged

Fix storage layer limit #11156

merged 2 commits into from
Apr 4, 2022

Conversation

athei
Copy link
Member

@athei athei commented Apr 3, 2022

Fixup for #10808

We cannot use atomics to store the storage layer counter because of the native runtime: In the native runtime multiple different calls into the runtime happen concurrently on the same memory. In the wasm case every call gets its own wasm instance with its own memory as it should be.

Storage access works as global data because the client uses thread local data for it.

@athei athei added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Apr 3, 2022
@xlc
Copy link
Contributor

xlc commented Apr 3, 2022

can we use atomics for wasm runtime only?

@shawntabrizi
Copy link
Member

can we use atomics for wasm runtime only?

AFAIK yes, but in this case, there is probably not a significant overhead between the two options here. I think using overlay storage is fine, and it will never commit anything to the runtime since it always resets.

@athei
Copy link
Member Author

athei commented Apr 4, 2022

Calling into the host should still be way slower than using atomics or environmental. But in this case it doesn't matter because it isn't a function that is called a lot of times.

I think a proper solution would be to pick up this PR and add some thread_local! storage in case of native. I might do that soon.

@athei
Copy link
Member Author

athei commented Apr 4, 2022

I don't get why the companion is failing. It seems unrelated. Was something missing from #11136 ?

@athei athei merged commit 0d88803 into master Apr 4, 2022
@athei athei deleted the at-fix-transactions-layers branch April 4, 2022 11:19
@jakoblell jakoblell added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jun 7, 2022
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants